Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Defaults content_type to application/octet-stream in blob_record.js #38124

Merged
merged 1 commit into from
Jan 7, 2020

Conversation

weilandia
Copy link
Contributor

Summary

Defaults content_type to application/octet-stream in blob_record.js in order to resolve #38123.

@cushingw recommended this solution here #36514 (comment).

Closes #38123

@weilandia weilandia changed the title Defaults content_type to application/octet-stream Defaults content_type to application/octet-stream in blob_record.js in order to resolve #38123 Dec 30, 2019
@weilandia weilandia changed the title Defaults content_type to application/octet-stream in blob_record.js in order to resolve #38123 Defaults content_type to application/octet-stream in blob_record.js Dec 30, 2019
@weilandia
Copy link
Contributor Author

weilandia commented Dec 30, 2019

Some other options:

Defaults content_type to application/octet-stream in blob_record.js

[direct_upload_xls_in_chrome]
@javan javan merged commit 11781d0 into rails:master Jan 7, 2020
@javan
Copy link
Contributor

javan commented Jan 7, 2020

Thanks!

@weilandia weilandia deleted the direct_upload_xls_in_chrome branch January 8, 2020 01:03
@weilandia
Copy link
Contributor Author

@javan I noticed https://contributors.rubyonrails.org wrongly credited this commit to the branch name because it was in square brackets in the commit. Very much not important, but wondering if there's a way to manually overwrite? Thanks

@javan
Copy link
Contributor

javan commented Jan 8, 2020

Wups! Opened rails/rails-contributors#146 to fix that up.

@javan
Copy link
Contributor

javan commented Jan 11, 2020

@weilandia
Copy link
Contributor Author

Thanks @javan!

@carlaraya
Copy link

It seems like the npm package (https://www.npmjs.com/package/@rails/activestorage) hasn't reflected the changes in this commit yet. The PR conversation is most likely not the right place to ask, but how can I implement this commit without waiting for the npm package to update?

smartygus added a commit to smartygus/activestorage that referenced this pull request Mar 4, 2020
inludes fix made here: rails/rails#38124

Otherwise it's identical to 6.0.2.1
smartygus added a commit to smartygus/activestorage that referenced this pull request Mar 4, 2020
inludes fix made here: rails/rails#38124

Otherwise it's identical to 6.0.2.1
@smartygus
Copy link
Contributor

It seems like the npm package (https://www.npmjs.com/package/@rails/activestorage) hasn't reflected the changes in this commit yet. The PR conversation is most likely not the right place to ask, but how can I implement this commit without waiting for the npm package to update?

@carlaraya I basically forked the Rails repo from the 6.0.2.1 release, cherry-picked the commit that made this fix, then extracted the activestorage subdirectory into another git repo to create a patched activestorage repo, which I could then use directly with yarn, because it supports adding from a git repo, but unfortunately not if the package is in a subdirectory of a repo (see here).

So in my case, adding the patched package with yarn became: yarn add https://github.com/smartygus/activestorage

javan added a commit that referenced this pull request Mar 9, 2020
Defaults content_type to application/octet-stream in blob_record.js
@javan
Copy link
Contributor

javan commented Mar 9, 2020

I just backported this in 2c19a6e so it'll go out in the next release.

Aketzu added a commit to kanijo-oy/Kanijo that referenced this pull request Mar 21, 2020
Aketzu added a commit to kanijo-oy/Kanijo that referenced this pull request Mar 21, 2020
@maurobender
Copy link

Great! Thank you so much!! I was just facing this issue after updating to rails 6.

When is the next update including this fix for the npm package planned to happen? The las update was 5 months ago.

@smartygus
Copy link
Contributor

@maurobender the package you linked to is no longer used for Rails 6 (for Rails 5 only it seems). You can find the updated package here: https://www.npmjs.com/package/@rails/activestorage

@maurobender
Copy link

@maurobender the package you linked to is no longer used for Rails 6 (for Rails 5 only it seems). You can find the updated package here: https://www.npmjs.com/package/@rails/activestorage

Thanks a lot @smartygus! Any chance of adding this to the guides (maybe here)?

@smartygus
Copy link
Contributor

Any chance of adding this to the guides (maybe here)?

@maurobender do you mean something to explain that the npm packages have moved? This is mentioned in the guide for upgrading from Rails 5.2 to Rails 6, and I think that's probably the most appropriate place to document such changes: https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#all-npm-packages-have-been-moved-to-the-@rails-scope
As far as I can tell, all mentions of the ActiveStorage npm package in the guides have also been updated to use the @rails/activestorage scoped package name, so I think the guides seem okay in terms of pointing people to the correct package.

Or did you have something else in mind?

@@ -550,7 +550,7 @@
this.file = file;
this.attributes = {
filename: file.name,
content_type: file.type,
content_type: file.type || "application/octet-stream",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it makes sense to set null: false for content_type in the migration, since it should never be NULL and causes errors if it is NULL?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds like a good idea. Would you like to make a PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: #43055

betesh added a commit to betesh/rails that referenced this pull request Aug 19, 2021
As of rails#38124 (Jan, 2020),
it has a default value so it will never be NULL.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants